-
Notifications
You must be signed in to change notification settings - Fork 592
Add boost.assign 1.83.0.bcr.1 #6543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add boost.assign 1.83.0.bcr.1 #6543
Conversation
|
@bazel-io skip_check unstable_url |
|
Hello @cdelguercio, modules you maintain (boost.assign) have been updated in this PR. |
|
Hello @Vertexwahn, modules you maintain (boost.assign, boost.timer) have been updated in this PR. |
|
Hello @vtsao-openai, modules you maintain (boost.timer) have been updated in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds version 1.83.0.bcr.1 for both boost.assign and boost.timer to the Bazel Central Registry. The changes for boost.timer correctly include test targets. My review has identified a few issues that need to be addressed to ensure correctness and maintainability. For boost.assign, a critical dependency is missing, and a test dependency should be marked as a development dependency. For boost.timer, several test-only dependencies also need to be marked as development dependencies. Please see the detailed comments for specific suggestions.
| deps = [ | ||
| "@boost.array", | ||
| "@boost.config", | ||
| "@boost.core", | ||
| "@boost.move", | ||
| "@boost.preprocessor", | ||
| "@boost.ptr_container", | ||
| "@boost.range", | ||
| "@boost.static_assert", | ||
| "@boost.throw_exception", | ||
| "@boost.tuple", | ||
| "@boost.type_traits", | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cc_library for boost.assign is missing a dependency on @boost.multi_index. According to the Boost documentation, boost.assign depends on boost.multi_index. Please add it to the deps list to ensure the module builds correctly for consumers.
deps = [
"@boost.array",
"@boost.config",
"@boost.core",
"@boost.move",
"@boost.multi_index",
"@boost.preprocessor",
"@boost.ptr_container",
"@boost.range",
"@boost.static_assert",
"@boost.throw_exception",
"@boost.tuple",
"@boost.type_traits",
],
| bazel_dep(name = "boost.ptr_container", version = "1.83.0.bcr.1") | ||
| bazel_dep(name = "boost.range", version = "1.83.0.bcr.1") | ||
| bazel_dep(name = "boost.static_assert", version = "1.83.0.bcr.1") | ||
| bazel_dep(name = "boost.test", version = "1.83.0.bcr.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency on boost.test is for running tests and is not a required dependency for the library itself. It should be marked as a development dependency by adding dev_dependency = True. This prevents downstream users from unnecessarily fetching test-only dependencies.
| bazel_dep(name = "boost.test", version = "1.83.0.bcr.1") | |
| bazel_dep(name = "boost.test", version = "1.83.0.bcr.1", dev_dependency = True) |
| bazel_dep(name = "boost.chrono", version = "1.83.0.bcr.1") | ||
| bazel_dep(name = "boost.core", version = "1.83.0.bcr.1") | ||
| bazel_dep(name = "boost.detail", version = "1.83.0.bcr.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependencies boost.chrono, boost.core, and boost.detail are only used for tests, as indicated by the comment and their usage in overlay/BUILD.bazel. They should be marked as development dependencies by adding dev_dependency = True to each. This prevents downstream users from unnecessarily fetching test-only dependencies.
| bazel_dep(name = "boost.chrono", version = "1.83.0.bcr.1") | |
| bazel_dep(name = "boost.core", version = "1.83.0.bcr.1") | |
| bazel_dep(name = "boost.detail", version = "1.83.0.bcr.1") | |
| bazel_dep(name = "boost.chrono", version = "1.83.0.bcr.1", dev_dependency = True) | |
| bazel_dep(name = "boost.core", version = "1.83.0.bcr.1", dev_dependency = True) | |
| bazel_dep(name = "boost.detail", version = "1.83.0.bcr.1", dev_dependency = True) |
bazel-io
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All modules in this PR have been approved by their maintainers. This PR will be merged if all presubmit checks pass.
Following structure added in 82895ad without using symlink for MODULE.bazel (#6440).
Adjusted according to other boost.* 1.83.0.bcr.1 targets:
Adding @bazel-io skip_check unstable_url as it doesn't have a stable url.
Ran and applied modifications produced by: